Parroting gateway names for now. This probably needs a review into valueSet or valueList, but the gateway names should be fine for right now

This test and others with no clause are just for us to ensure that we're typing things correctly.

I use inheritance here to make sure we enforce what methods are available and to reduce duplication

This object set only has filter and pivot as aggregations and selectProperty is not available on the base object set.

BaseObjectSet extends SingleLinkWithPropertyObjectSet. This makes the selectProperty method available only for chains of single links, which correlates with the backend. The pivotTo operation in BaseObjectSet enforces that if we pivot to a many link, we from then on use ManyLinkWithPropertyObjectSet which does not have selectProperty available and only returns further ManyLink object sets.

This is all we need to propagate the types through all further object set operations. The FLUP here is to make the type of the whole object set extractable and definable so that a user can pass around object sets that are correctly typed with the additional properties and do not have to be inferred.

So just clarifying, the reason we need to duplicate pivotTo here is because if there's a many link anywhere in the clause, you can't select anymore? Even if you go from a many to a single link?

Yeah that's correct, which makes sense because for this to work you have to basically be navigating to exactly one object

We should probably document that here too.

Does this have to extend FilterableDeriveObjectSet<Q> purely to make ManyLinkWithPropertyObjectSet work? If so, could we just have that extend FilterableDeriveObjectSet<Q> as well? Feels a bit confusing the way its setup now.

Not a huge deal though

Fixed! Agreed this makes it more readable

nit: for this and the type below can we call them something like CollectDeriveAggregations, really want to make sure we don't confuse these with existing normal agg options

Debating between the above Derive, ow WithPropertyBaseAggregations, or BaseWithPropertyAggregations. We aren't using "derive" as a term, but it is much cleaner than the other 2. Number 3 is kind of confusing, but number 2 would mean they al start the same which I don't want. Stuck with number 1 for conciseness

Can we not narrow this type down at all?

We can type this down to a string, I was wondering if we should type this as unknown to tell the user -> dont touch this

Typed it as a string even though we could use a number in case we decide we want to use a UUD or something else in the future

If we want to keep all the agg options a bit simpler to parse though, another way to format this type could be keeping the Agg_For_Type and modifying it to take an extra boolean (that controls whether this is for derive or not, default to false) and have it branch to the right agg options that way, rather than having the responsibility on the user to pass in custom options if they want. That being said, if you feel like in the future we'll need to customize agg options even more, this does make it more flexible so up to you

If you do keep it like this, lets remove AGG_FOR_TYPE if its not being used anywhere

Hmm yeah this does make sense to me, will refactor

is as any the best we can do here?

Looks like we don't need it, this was left over from dev

me being annoying nit: but for completeness, lets add an empty string arg as well and make sure that errors

Can we add a test to verify things don't break if we select a property that has all undefined values?

What do you mean?

Basically if a prop returns an undefined value

Can we also add a test to make sure that within a WithPropertiesClause you can't just return something like base.pivotTo("lead") without doing some sort of selection or aggregation?

Apologies in advance if I missed this somewhere, but don't seem to see that being tested anywhere yet

This is guarded by the type of the lambda, but I will add an explicit test for this behavior

@ssanjay1 Is this what you had in mind?

Yea that works

Though maybe keeping parity with the other names it should be aggregate | derivedPropertiesAggregate or something along those lines, basically including the word derive

nit: rename U so its clear what that boolean generic is for

Though maybe keeping parity with the other names it should be aggregate | derivedPropertiesAggregate or something along those lines, basically including the word derive

Lets be consistent with naming, see some withProperty and derive here.

If I am thinking about whats ideal we need to add some type tests. I'm okay if its in a follow up

I'd have to check but my gut is keyof D & string is better for the compiler than the conditional type that Extract causes.

I'm okay with it for now but this feels like a place where having the simpler types that are not based on functions produces more readable types? We have to do the extraction work anyway to create things like properties and props below but we are required to keep the function hierarchy in the type generics as this object is passed around instead of resolving them at the time withProperties is called and thus carrying a lot less. Like we could just be carrying { foo: number, bar: string } lets say. Or even { foo: PropertyDefinition, bar: PropertyDefinition }.

Extra new line causes grouping when organizing imports instead of perfect sorting.

I would want to see some expectTypeOf entries for all of this. It both makes it easier to review and gives protections against the future that we didn't break things. Right now we just know that the incoming syntax is correct but not the output.

For something like this, especially so you can add type tests you want this to be more like const clause = { ... } satisfies WithPropertiesClause<Employee> so that it will capture the raw values

Nit: don't pass this string. This test seems to be directly testing createWithPropertiesObjectSet and so you can literally pass a reference to said function.

I think this also means that this entire describe should really be in createWithPropertiesObjectSet.test.ts so its clearer whats under test.

I have some mixed feelings about the definitionId still, especially considering it ends up on the public api.

Suppose for a moment definitionMap is actually Map<object, DerivedPropertyDefinition>.

instead of definitionId we could do this:

const ret = {} as WithPropertyDefinition<any>;
definitionMap.set(ret, {... });
return ret;

No need to do accounting of ids and we don't have to leak them/make them part of our public API.

Its still not perfect because people might return their own objects thinking thats okay when its not and they still might do something weird like reuse an object from before (which won't work because its a different map) but its closer.

Ultimately I think we are probably better just designing an IR that is based on raw objects.